Skip to content

RoamingSettingsHelper with initial support for UserExtensionsDataStore#77

Merged
shweaver-MSFT merged 37 commits intodevfrom
shweaver/roaming-settings
Apr 2, 2021
Merged

RoamingSettingsHelper with initial support for UserExtensionsDataStore#77
shweaver-MSFT merged 37 commits intodevfrom
shweaver/roaming-settings

Conversation

@shweaver-MSFT
Copy link
Member

@shweaver-MSFT shweaver-MSFT commented Mar 10, 2021 •
edited
Loading

Fixes #72
Fixes #73
Fixed #84

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

RoamingSettings has been deprecated by the platform. We need a replacement and the Graph works great for this.

What is the new behavior?

I've added a new RoamingSettingsHelper class that extends IObjectStorageHelper. When constructed, a RoamingDataStore enum value is passed in to determine which Graph based storage method to use. The developer can then interact with the helper instance to perform CRUD operations on the roaming settings key/value pairs.

/// <summary>
/// An enumeration of the available data storage methods for roaming data.
/// </summary>
public enum RoamingDataStore
{
    /// <summary>
    /// Store data using open extensions on the Graph User.
    /// </summary>
    UserExtensions,
}

// RoamingDataStore.UserExtensions is the default
var roamingSettings = await RoamingSettingsHelper.GetForCurrentUser();

// Get a value from the store
string value = roamingSettings.Read<string>(key);

// Set a value to the store.
// Keys cannot be deleted, only added or updated.
roamingSettings.Save(key, value);

// Create the custom user extension for storing data to.
var newExtension = await roamingSettings.Create();

// Delete the custom user extension. This is the only way to clear keys.
await roamingSettings.Delete();

// Update internal keys and values with fresh data from the Graph.
await roamingSettings.Sync();

Their are additional parameters that can be used to configure the RoamingSettingsHelper:

  • SyncOnInit - Determines if the data should automatically sync immediately upon creation of the helper instance. Default true.
  • AutoSync - Determines if the data should sync on every transaction or wait until the Sync method is called explicitly. Default true.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

You must be signed in with the Msal provider for this to work. The mock endpoint will fail.

@ghost
Copy link

ghost commented Mar 10, 2021

Thanks shweaver-MSFT for opening a Pull Request! The reviewers will test the PR and highlight if there is any merge conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost assigned michael-hawker Mar 10, 2021
@shweaver-MSFT shweaver-MSFT marked this pull request as ready for review March 12, 2021 20:38
@shweaver-MSFT shweaver-MSFT changed the title DRAFT: RoamingSettingsHelper with initial support for UserExtensionsDataStore RoamingSettingsHelper with initial support for UserExtensionsDataStore Mar 12, 2021
@shweaver-MSFT
Copy link
Member Author

shweaver-MSFT commented Mar 12, 2021

@nmetulev I've made some changes to simplify the usage based on our last chat.

I wasn't able to reduce the constructor parameters to zero, but I do have a method that makes it just as simple. The issue is that I need to make an async request to the Graph to get the current user for their id. That can't be done in the constructor of the helper, and I don't like mandating Initialize() patterns. Also most of the methods from IObjectStorageHelper are synchronous, so I need some opportunity to load the current user after sign in, but before construction of the helper. I chose to have a static async function that returns the helper configured for the current user:

var roamingSettings = await RoamingSettingsHelper.GetForCurrentUser();

What do you think? Interested to know if you have any other ideas for this.

@shweaver-MSFT shweaver-MSFT changed the base branch from main to dev March 18, 2021 23:03
@nmetulev
Copy link
Contributor

nmetulev commented Mar 25, 2021

I think there might be something wrong with this PR, I only see unit test changes and no other code

image

@shweaver-MSFT
Copy link
Member Author

shweaver-MSFT commented Mar 25, 2021 •
edited
Loading

@nmetulev I goofed this royally! Looks I forgot to change branches when I manually merged the changes from the package shuffle. I'll fix it

@shweaver-MSFT
Copy link
Member Author

shweaver-MSFT commented Mar 25, 2021

@nmetulev, I fixed up the bad merge. Sorry about that :/

nmetulev
nmetulev reviewed Mar 25, 2021
View reviewed changes
Copy link
Contributor

@nmetulev nmetulev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome

What do you think about moving the RoamingSettingsHelper to the Graph package instead of the Controls package? Is there a dependency to UWP?

CommunityToolkit.Uwp.Graph.Controls/Helpers/RoamingSettings/RoamingSettingsHelper.cs Outdated
get => DataStore.Read<object>(key);
set
{
if (DataStore.KeyExists(key))
Copy link
Contributor

@nmetulev nmetulev Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it doesn't exist, shouldn't it create the key?

Copy link
Member Author

@shweaver-MSFT shweaver-MSFT Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, good point. I think it should.

Copy link
Contributor

@nmetulev nmetulev Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - will you do that in this PR?

Copy link
Member Author

@shweaver-MSFT shweaver-MSFT Apr 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

@nmetulev nmetulev mentioned this pull request Mar 25, 2021
Closed
@shweaver-MSFT
Copy link
Member Author

shweaver-MSFT commented Mar 25, 2021

Awesome

What do you think about moving the RoamingSettingsHelper to the Graph package instead of the Controls package? Is there a dependency to UWP?

In spirit I 100% agree. The thing that gets in the way is the IObjectStorageHelper coming from the Microsoft.Toolkit.Uwp package. I can't put that in the CommunityToolkit.Net.Graph package because it is NetStandard.

I'm losing sight of why I'm extending the IObjectStorageHelper to begin with. It seemed like a good idea at first, but I'm not sure if it's really the right thing to do...

@michael-hawker, what do you think? Do you have any strong convictions on why we should stick with IObjectStorageHelper, compared to the limitations (forcing UWP dependency for one)?

@shweaver-MSFT
Copy link
Member Author

shweaver-MSFT commented Mar 30, 2021

I've created a new issue on WCT repo to migrate the ObjectStorage interfaces up to the *.Toolkit package to remove the Windows dependency. CommunityToolkit/WindowsCommunityToolkit#3903

Let's review this for now so I can get the OneDrive PR up next. Once the work is done in WCT, I'll update and migrate the RoamingSettings logic up to the *.Graph package, and out of the UWP package.

@shweaver-MSFT shweaver-MSFT requested a review from nmetulev March 30, 2021 20:34
nmetulev
nmetulev approved these changes Apr 1, 2021
View reviewed changes
Copy link
Contributor

@nmetulev nmetulev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to merge after addressing last comment

@shweaver-MSFT
Copy link
Member Author

shweaver-MSFT commented Apr 2, 2021

Fixed the last comment, merging now 🚀

@shweaver-MSFT shweaver-MSFT merged commit 326cf67 into dev Apr 2, 2021
@shweaver-MSFT shweaver-MSFT deleted the shweaver/roaming-settings branch April 2, 2021 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nmetulev nmetulev nmetulev approved these changes

@michael-hawker michael-hawker Awaiting requested review from michael-hawker

Assignees

@shweaver-MSFT shweaver-MSFT

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature] Enable roaming settings through open extension on the Graph User [Feature] Create a system for RoamingSettings via the Graph

3 participants

@shweaver-MSFT @nmetulev @michael-hawker